Skip to content

feat: create credential providers before synthesizing a deploy - #2123

Open
notgitika wants to merge 16 commits into
refactorfrom
feat/deploy-credential-providers
Open

feat: create credential providers before synthesizing a deploy#2123
notgitika wants to merge 16 commits into
refactorfrom
feat/deploy-credential-providers

Conversation

@notgitika

@notgitika notgitika commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Creates or updates a project's credential providers before synthesis, so the synthesized CDK app can read their ARNs out of deployed-state.json. Without this, deploying a project that declares any credential fails inside cdk synth.

What it does

  • Between the account preflight and synth, CdkBackend.deploy provisions each declared credential provider and records the ARNs via updateTargetState. Local prerequisites are checked first, so a setup error never mutates AWS.
  • Created when absent, updated when present, so editing a secret and redeploying pushes the new value (as main does). A credential with no secret the CLI can see leaves an existing provider untouched instead of failing.
  • Secrets come from .env.local (AGENTCORE_CREDENTIAL_<NAME>) or a Secrets Manager secretRef; process-environment variables override the file, so CI needs no secrets on disk.
  • Payment credentials are provisioned (CoinbaseCDP, StripePrivy) and deleted on teardown. Deploy previously refused them, so add credentials payment could build a project that couldn't deploy.
  • A failure part-way deletes the providers that run created — never a pre-existing one — before rethrowing.
  • Runs through the existing core.identity client instead of a second SDK client; the target's credentials travel via CoreOptions.

Notes

  • Behaviour change from this PR's first revision: a redeploy now updates an existing provider, so it overwrites a secret rotated outside the CLI. The old "never update" behaviour meant an edited .env.local silently had no effect.
  • CoreClient's client cache is now keyed by credential identity — credentials are a function, which JSON.stringify drops, so different credentials in one region shared a cached client.
  • Credential names ending in a field suffix (_CLIENT_ID, _APP_SECRET, …) are refused at add time; their variable would shadow another credential's field. main guarded this; the rewrite had lost it.
  • Follow-up in @aws/agentcore-cdk: its API-key Gateway path doesn't grant GetSecretValue on an external secret ARN, so an API-key secretRef deploys and then fails at retrieval. The CLI already records the ARN.

Tested

  • Unit: create/update/reuse per credential type, environment override, missing-secret errors, legacy _CLIENT_ID fallback, payment vendors, teardown deletion, rollback.
  • e2e (us-west-2), first revision: add credentials api-keydeploy created the provider before synth, wrote it to deployed-state.json, then merged stackArn into the same entry.

@github-actions github-actions Bot added the size/l PR size: L label Aug 27, 2026
@agentcore-devx-automation agentcore-devx-automation Bot added the agentcore-harness-reviewing AgentCore Harness review in progress label Aug 27, 2026

@agentcore-devx-automation agentcore-devx-automation Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AgentCore Harness Review

Verdict: Looks good

Nice PR. The design of running credential provisioning before cdk synth and threading the ARNs through deployed-state.json is well-motivated and the comments do a great job of capturing why. The seam boundary (IdentityProviderClient) is drawn at the SDK client rather than at fs/process boundaries, so the tests avoid excessive mocking while still exercising the real spec + .env.local parsing paths. Sharing credentialEnvVarName/CLIENT_SECRET_SUFFIX between add and deploy via envLocal.ts (with a re-export from shared.ts) removes a latent format-drift bug.

A couple of small things that aren't blockers but worth confirming intentional:

  • Stale credential entries when the spec goes to zero credentials. In src/core/project/backends/cdk.ts (~L115) updateTargetState is only called when Object.keys(provisioned).length > 0. If a user deletes their last credential from agentcore.json and re-deploys, provisioned is {}, the state write is skipped, and the previous resources.credentials map is left on disk. The doc-comment on updateTargetState promises "A resource map provided in the patch replaces the previous map for that kind wholesale, so a credential dropped from the spec stops being advertised" — that guarantee is only actually delivered when at least one credential remains. Since the synthesized CDK app looks up credentials by name, this is likely inert in practice, but if you want the drop-to-zero case to behave the same as drop-one-of-many, you'd either always call updateTargetState({ resources: { credentials: provisioned } }) or explicitly write {} when declared is non-empty on the spec side but you provisioned nothing.

  • parseEnv cast in EnvLocalFile.read (src/core/project/envLocal.ts L90): parseEnv's declared return type is Record<string, string | undefined> (last-write-wins across duplicate keys), but you cast to Record<string, string>. All callers happen to use if (!value) so undefined is handled safely today; just be aware the type is a small lie and a future caller doing env[k].trim() would compile but crash.

Neither of these needs to block the merge.

@agentcore-devx-automation agentcore-devx-automation Bot removed the agentcore-harness-reviewing AgentCore Harness review in progress label Aug 27, 2026
@notgitika
notgitika force-pushed the feat/deploy-credential-providers branch from 4b787a8 to 0db4266 Compare August 27, 2026 15:43
@github-actions github-actions Bot added size/l PR size: L and removed size/l PR size: L labels Aug 27, 2026
Base automatically changed from feat/deployed-state-top-level to refactor August 27, 2026 16:15
@github-actions github-actions Bot added size/xl PR size: XL and removed size/l PR size: L labels Aug 27, 2026
The synthesized CDK app reads credential provider ARNs out of
deployed-state.json and fails to synth a project that declares credentials
until they exist. Provision them between the account preflight and the
build, then record their ARNs via updateTargetState so the assembly is
synthesized against a state file that already describes them.

Providers are created when absent and reused when present, never updated,
so a redeploy neither mints a new secret version nor overwrites one rotated
outside the CLI. Payment credentials are rejected up front (agentcore.json
can't express the vendor config they need). Secrets come from the same
place 'project add credentials' writes them, so the env-var name is now
derived from one function in envLocal.ts that both sides share.
@notgitika
notgitika force-pushed the feat/deploy-credential-providers branch from 0db4266 to 3c09c23 Compare August 27, 2026 16:28
@github-actions github-actions Bot added size/l PR size: L and removed size/xl PR size: XL size/l PR size: L labels Aug 27, 2026
@agentcore-devx-automation agentcore-devx-automation Bot added the claude-security-reviewing Claude Code /security-review in progress label Aug 27, 2026
@codecov-commenter

codecov-commenter commented Aug 27, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.36207% with 54 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.10%. Comparing base (d304147) to head (35a3a20).

Files with missing lines Patch % Lines
src/core/identity.tsx 12.82% 34 Missing ⚠️
src/core/project/backends/cdk/credentials.ts 96.08% 14 Missing ⚠️
src/core/index.tsx 53.84% 6 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           refactor    #2123      +/-   ##
============================================
- Coverage     97.22%   97.10%   -0.13%     
============================================
  Files           507      508       +1     
  Lines         33809    34269     +460     
============================================
+ Hits          32872    33276     +404     
- Misses          937      993      +56     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@agentcore-devx-automation

Copy link
Copy Markdown
Contributor

Claude Security Review: no high-confidence findings. (run)

@agentcore-devx-automation agentcore-devx-automation Bot removed the claude-security-reviewing Claude Code /security-review in progress label Aug 27, 2026
@notgitika
notgitika marked this pull request as ready for review August 27, 2026 16:38
@github-actions github-actions Bot added size/l PR size: L and removed size/l PR size: L labels Aug 27, 2026
…v type

- Add SDK-mocked coverage for createIdentityProviderClient (the real Identity
  factory the provisioner tests bypass): ~55% -> ~95% on credentials.ts.
- Always record the provisioned credential set, so removing the last credential
  from the spec clears the stale entry instead of leaving it advertised.
- EnvLocalFile.read returns Record<string, string | undefined> (parseEnv's real
  type) rather than casting it away.
- Tighten a few verbose comments.
@github-actions github-actions Bot added size/l PR size: L and removed size/l PR size: L labels Aug 27, 2026
@agentcore-devx-automation agentcore-devx-automation Bot added the claude-security-reviewing Claude Code /security-review in progress label Aug 27, 2026
@agentcore-devx-automation

Copy link
Copy Markdown
Contributor

Claude Security Review: no high-confidence findings. (run)

@github-actions github-actions Bot added size/xl PR size: XL and removed size/xl PR size: XL labels Aug 28, 2026
@agentcore-devx-automation agentcore-devx-automation Bot added the claude-security-reviewing Claude Code /security-review in progress label Aug 28, 2026
@agentcore-devx-automation

Copy link
Copy Markdown
Contributor

Claude Security Review: no high-confidence findings. (run)

@agentcore-devx-automation agentcore-devx-automation Bot removed the claude-security-reviewing Claude Code /security-review in progress label Aug 28, 2026
? { name: credential.name, secretRef: credential.secretRef }
: {
name: credential.name,
apiKey: requireEnvSecret(credential.name, env, rootPath, "secretRef"),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we already have all the code we need to do this in the existing core package. Let's reuse that. We can discuss strategies for making that code reusable if it's not clear.

aidandaly24
aidandaly24 previously approved these changes Aug 31, 2026
gitikavj added 6 commits August 31, 2026 21:49
Deploy's credential provisioning built its own BedrockAgentCoreControlClient
and sent Get/Create commands itself, duplicating the IdentityClient in
src/core/identity.tsx that already backs the `agentcore identity` commands.
It existed only because the credentials had nowhere to travel: control-plane
clients were built from a ClientConfig that carried no credentials, while
provisioning must run against the deployment target's own.

- CoreOptions and ClientConfig now carry optional credentials, forwarded by
  toClientConfig, mirroring the CredentialedClientConfig the CloudFormation
  factory already uses.
- CoreClient hands its IdentityClient to FsProjectManager, which passes it to
  CdkBackend; the provisioner takes CredentialProviderCalls, a four-method
  Pick of CoreIdentityClient, so tests fake four calls instead of ten.
- cacheKey no longer keys clients by JSON.stringify alone: credentials are a
  provider function, which JSON.stringify drops, so two callers with
  different credentials in one region would have shared a cached client.
  They are keyed by object identity instead.
- IdentityClient's dependency narrows to Pick<AwsClients, "control">, letting
  a project manager built outside CoreClient construct one from the existing
  createControlClient factory.

The lazily imported SDK is gone with the duplicate client; the claim that it
saved startup cost was already untrue, since core/factories.tsx imports the
same client statically on every run. credentials.client.test.ts existed only
to mock that import and is deleted; its response-mapping and not-found cases
move into credentials.test.ts.
The existing collision check compares the .env.local variables the credentials
in the spec write today, so it cannot catch a clash with a field the CLI no
longer writes but still reads — an OAuth client id, which pre-0.29 projects
keep in AGENTCORE_CREDENTIAL_<NAME>_CLIENT_ID — or with one a later credential
type adds. An api-key credential named 'svc-client-id' therefore added cleanly
and its key was then read as the client id of an OAuth credential named 'svc'.

Names whose derived variable ends in a field suffix are now refused at add
time, the way main's validateCredentialNameEncryptable refuses them. The check
stays in the add flow rather than the schema so that a project already holding
such a name keeps loading and can be repaired.
…ider

Deploy reused an existing provider untouched, so editing a secret in
.env.local and redeploying had no effect on AWS: the provider kept the value
it was created with, and nothing said so. main's deploy updates instead —
`// Always update to ensure provider has current credentials` — and losing that
on the rewrite would be a silent regression for anyone rotating a key.

A credential whose secret the CLI can see is now written on every deploy:
created when the provider is absent, updated when it is present. A credential
with no secret to offer — nothing in .env.local and no external reference —
leaves an existing provider exactly as it is, so a project that provisioned
once and no longer keeps the secret on disk still deploys; only an absent
provider fails, as before.
Provisioning read agentcore/.env.local and nothing else, so a deploy from CI
had to write its secrets to disk first. main reads every credential variable
from process.env and merges it over the file, which is what makes a
non-interactive deploy possible without persisting secrets.

Variables carrying the credential prefix now override the file. The filter
keeps the deploy from reading anything else out of the environment, and the
provisioner takes the environment as an argument so tests do not mutate the
process's own.
`project add credentials payment` and `add payment-connector` write a payment
credential to the spec, but deploy refused to provision one — it threw and told
the user to remove it — so a project could be assembled that added cleanly and
then could not deploy at all. main provisions them, and the CDK construct that
wires payment connectors already reads their ARNs out of the credentials map in
deployed-state.json.

- Core's Identity client gains the payment provider operations. The pinned SDK
  carries them, so unlike main there is no hand-signed HTTP request.
- A payment credential is created when absent and updated when present, from
  the vendor's variables: CoinbaseCDP's api key id, api key secret and wallet
  secret, or StripePrivy's app id, app secret, authorization private key and
  authorization id. Every variable that is unset is named in one error rather
  than one per attempt, and an existing provider is left alone when they are
  all absent.
- Teardown deletes the payment providers the project declares, as main's
  cleanupPaymentCredentialProviders does, after the stack rather than before,
  since a resource in it may still be using one. Only payment providers: an
  api-key or OAuth provider is named account-globally and may be shared with
  another project.

The payment collision test now asserts what actually guards that case: a name
ending in a payment field suffix is refused on its own, so a credential can no
longer be created that would collide with a payment credential's variables.
Resolving every credential before writing any removes the likeliest cause of a
half-provisioned deploy — a missing secret — but not the rest: a create that
fails on throttling or permissions after an earlier one succeeded left a
provider in AWS that deployed-state.json never recorded. Retrying adopted it by
name, but abandoning the deploy or dropping the credential orphaned it.

A failure during the write loop now deletes the providers that same run created,
newest first, and rethrows the original error. A provider that already existed
is not deleted: this deploy only updated its secret, and undoing that would need
the value it held before, which the CLI never had. A deletion that fails is
reported — naming the provider and saying the next deploy will adopt it —
rather than replacing the error that stopped the deploy.
@github-actions github-actions Bot added size/xl PR size: XL and removed size/xl PR size: XL labels Aug 31, 2026
@agentcore-devx-automation agentcore-devx-automation Bot added the claude-security-reviewing Claude Code /security-review in progress label Aug 31, 2026
@github-actions github-actions Bot added size/xl PR size: XL and removed size/xl PR size: XL labels Aug 31, 2026
@agentcore-devx-automation

Copy link
Copy Markdown
Contributor

Claude Security Review: no high-confidence findings. (run)

@agentcore-devx-automation agentcore-devx-automation Bot removed the claude-security-reviewing Claude Code /security-review in progress label Aug 31, 2026
Conflicts were additive on both sides:
- envLocal: this branch added `read`, refactor added `removeKeys`; both kept.
- CdkBackend: this branch added the credential provisioner and payment remover,
  refactor replaced the stack probe with `describeStack`; both kept, and
  teardown now asks `describeStack` whether the stack is still there.
@github-actions github-actions Bot added size/xl PR size: XL and removed size/xl PR size: XL labels Aug 31, 2026
@agentcore-devx-automation

Copy link
Copy Markdown
Contributor

Claude Security Review: no high-confidence findings. (run)

@notgitika

Copy link
Copy Markdown
Contributor Author

e2e run — account 998846730471, us-west-2

Ran the full flow against a real account after the merge with refactor. Every scenario below passed; the account is clean afterwards (no providers, no stacks).

# Scenario Result
1 First deploy creates the provider before synth Preparing credential provider 'e2ekey2' precedes Synthesizing CloudFormation templates; provider ARN + secret ARN written to deployed-state.json, then stackArn merged into the same target entry
2 Rotate the secret in .env.local, redeploy Provider updated: new AWSCURRENT secret version at 22:59:05 against a create at 22:57:22, and GetSecretValue returned the rotated value
3 Variable in the process environment .env.local held one value, the environment another → the environment's value is what landed in AWS
4 No secret in the file or the environment Deploy succeeded, no new secret version, stored value unchanged — the existing provider is left alone rather than failing the deploy
5 Second credential fails after the first is created Removing credential provider 'rbkeya' this deploy created; rbkeya gone from AWS, pre-existing e2ekey2 untouched, deployed-state.json unchanged, exit code 1 with the original service error
6 Payment credential (CoinbaseCDP) paymentcredentialprovider/e2epay created and its ARN recorded. With two of three variables unset, one error named both: missing the values its payment provider needs: ..._API_KEY_SECRET, ..._WALLET_SECRET
7 Payment update, then teardown Rotated apiKeyId reached AWS on redeploy. Teardown removed the stack, deleted e2epay, kept e2ekey2 (account-global, may be shared), and emptied targets

Two notes for anyone re-running this:

  • The service validates vendor key formats. Placeholder payment secrets are rejected (Invalid apiKeySecret format: Expected base64-encoded Ed25519 private key, then Invalid walletSecret format: Expected base64-encoded EC P-256 private key). I used structurally valid throwaway keys, so create/update/delete and the request shape are verified — but not against real Coinbase credentials. Those service messages surface as-is rather than CLI-framed.
  • lastUpdatedTime on an API-key provider does not move on update, so get-api-key-credential-provider alone can't confirm a rotation; Secrets Manager version history can.

Unrelated papercut noticed on the way: project remove harness <name> fails with "too many arguments" — it wants --name <name>.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/xl PR size: XL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants